Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: [M3-8331] - useToastNotification async toasts #10841

Conversation

pmakode-akamai
Copy link
Contributor

@pmakode-akamai pmakode-akamai commented Aug 27, 2024

Description 📝

const toasts: Toasts = { ... }  should be moved to the event directory and map to event messages already defined in our new factory.

Changes 🔄

  • Refactor useToastNotification.
  • Move const toasts: Toasts = { ... } to Events/asyncToasts.tsx.
  • Map to event messages already defined in our new factory.
  • Update the event messages in Events/factories/disk.tsx.
  • Update e2e test cases.
  • Add unit test cases.

Target release date 🗓️

N/A

How to test 🧪

Verification steps

  • Verify all the async toasts continue to work as expected.
  • Confirm that the formatting and display of messages adhere to the event messages defined in our new Events/factories
  • Verify that all the test cases pass.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@pmakode-akamai pmakode-akamai self-assigned this Aug 27, 2024
@pmakode-akamai pmakode-akamai requested a review from a team as a code owner August 27, 2024 12:46
@pmakode-akamai pmakode-akamai requested review from carrillo-erik and coliu-akamai and removed request for a team August 27, 2024 12:46
@pmakode-akamai pmakode-akamai marked this pull request as draft August 27, 2024 12:46
Copy link

github-actions bot commented Aug 27, 2024

Coverage Report:
Base Coverage: 86.2%
Current Coverage: 86.2%

@pmakode-akamai pmakode-akamai marked this pull request as ready for review August 28, 2024 09:23
@pmakode-akamai pmakode-akamai force-pushed the M3-8331-refactor-useToastNotification-async-toasts branch from 28db55a to a655ac4 Compare August 28, 2024 09:25
@pmakode-akamai pmakode-akamai requested a review from a team as a code owner August 28, 2024 11:10
@pmakode-akamai pmakode-akamai requested review from AzureLatte and removed request for a team August 28, 2024 11:10
@jdamore-linode
Copy link
Contributor

@pmakode-akamai Hey Purvesh, just a heads up that this PR seems to be causing a handful of test failures -- at a glance they seem to be caused by toast notifications for certain events not appearing when Cloud expects them to appear

@pmakode-akamai pmakode-akamai marked this pull request as draft August 28, 2024 15:28
@pmakode-akamai
Copy link
Contributor Author

pmakode-akamai commented Aug 28, 2024

@pmakode-akamai Hey Purvesh, just a heads up that this PR seems to be causing a handful of test failures -- at a glance they seem to be caused by toast notifications for certain events not appearing when Cloud expects them to appear

Hey Joe (@jdamore-linode), this PR is still WIP. I’ll be looking into the test failures related to the toast notifications and will address them soon. Thanks!

@pmakode-akamai pmakode-akamai force-pushed the M3-8331-refactor-useToastNotification-async-toasts branch from 0d10cf4 to 908d1ce Compare August 29, 2024 07:27
@pmakode-akamai pmakode-akamai force-pushed the M3-8331-refactor-useToastNotification-async-toasts branch 2 times, most recently from 9af075e to 3f008f4 Compare September 2, 2024 07:39
@pmakode-akamai pmakode-akamai marked this pull request as ready for review September 2, 2024 12:20
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the changes - API is much cleaner now and implementation looks good and well test. Approving pending a few improvements and fixes

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Sep 3, 2024
Copy link
Contributor

@AzureLatte AzureLatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup! 🎉

✅ confirmed tests pass
✅ confirmed event messaging
✅ confirmed toasts for the flows mentioned
** caveat - I mainly tested for success toasts and don't really know how to trigger a lot of the failure toasts. However, I did see the snapshot failed toast + ran machine-image-upload.spec.ts a couple of times to see the failed toast for image upload ... will be trying to trigger the rest of the failure toasts 🤞

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 4, 2024
@pmakode-akamai pmakode-akamai force-pushed the M3-8331-refactor-useToastNotification-async-toasts branch from 403b862 to 1e74206 Compare September 5, 2024 09:46
@pmakode-akamai pmakode-akamai merged commit 766273a into linode:develop Sep 6, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants